[ES-1436915] Add additional null check for connection-context#810
Conversation
|
|
Not needed because the null checks exist in the later execution flow when logs are being exported. If we introduce more null checks, the issue at hand is masked more deeply which is inappropriate |
| .exportEvent(telemetryFrontendLog); | ||
| // Though we already handle null connectionContext in the downstream implementation, | ||
| // we are adding this check for extra sanity | ||
| if (connectionContext != null) { |
There was a problem hiding this comment.
we should introduce this weird ad-hoc check somewhere deep in the code when we are actually exporting the logs. This is happening already. So the PR is a no-op
There was a problem hiding this comment.
Yeah, we are already doing that. It is just that to make this future proof, where we accidently add dependency on connectionContext in any downstream logic and breaking the functionality. Telemetry is common to all executions. In current impl you're right that this is a no-op.
Telemetry should not block any critical executions. It is okay to mask nulls here. Also, null checks happen in other multiple sub-methods downstream, but this makes the check at a single entry point. |
jayantsing-db
left a comment
jayantsing-db
left a comment
There was a problem hiding this comment.
Discussed offline
| .setEntry(new FrontendLogEntry().setSqlDriverLog(telemetryEvent)); | ||
| TelemetryClientFactory.getInstance() | ||
| .getTelemetryClient( | ||
| connectionContext, DatabricksThreadContextHolder.getDatabricksConfig()) |
There was a problem hiding this comment.
should we check DatabricksConfig too? DatabricksThreadContextHolder.getDatabricksConfig
There was a problem hiding this comment.
Needed for authenticated client. Added the check there.
3 participants
Description
Add additional null check for connection context in TelemetryHelper. To clarify, this is a no-op change as we already handle null checks in further methods in downstream implementation. This change just makes it future proof for any further changes which may ignore null check in any of further business logic.
Testing
Ran all unit tests
Additional Notes to the Reviewer